Background: Use GLib.File instead of dealing with Paths #1711
Conversation
Simplify the code and avoids to deal with File paths directly.
lenemter
left a comment
lenemter
left a comment
There was a problem hiding this comment.
Can we omit the GLib namespace? It makes some lines pretty long.
|
I personally really prefer having the GLib namespace specified (when it's not a native type like string/int/bool) so that we know that it is not an object from the project but from GLib |
| public string filename { get; construct; } | ||
| public string[] key_frame_files { get; private set; default = {}; } | ||
| public GLib.File file { get; construct; } | ||
| public GLib.GenericArray<GLib.File> key_frame_files { get; private set; default = new GLib.GenericArray<GLib.File> (); } |
There was a problem hiding this comment.
Does this need to be a GenericArray? given that we known that the max number of files we will use is 2, isn't a fixed array enough.
| } | ||
|
|
||
| this.key_frame_files = key_frame_files; | ||
| this.key_frame_files = new_key_frame_files; |
There was a problem hiding this comment.
i don't think the this accessor is needed anymore here.
| background.set_data<unowned Background> ("delegate", this); | ||
|
|
||
| file_watches = new Gee.HashMap<string,ulong> (); | ||
| file_watches = new Gee.HashMap<GLib.File, ulong> (); |
There was a problem hiding this comment.
Use GLib.File.hash and GLib.File.equal here? Gee will fallback to direct_{hash,equal} if the functions aren't provided here.
| if (file_watches.has_key (file)) | ||
| return; |
There was a problem hiding this comment.
Code style.
| if (file_watches.has_key (file)) | |
| return; | |
| if (file_watches.has_key (file)) { | |
| return; | |
| } |
| else | ||
| load_image (filename); | ||
| private inline void load_file (GLib.File file) { | ||
| if (file.get_basename ().has_suffix (".xml")) { |
There was a problem hiding this comment.
i think we should use GLib.ContentType.guess() here, so that we can identify xml files without a extension. like the ones that the Gtk portal provides.
| // size of the monitor; for other backgrounds we can use the | ||
| // same background object for all monitors. | ||
| if (filename == null || !filename.has_suffix (".xml")) | ||
| if (file == null || !file.get_basename ().has_suffix (".xml")) |
There was a problem hiding this comment.
Same thing here, should check the mimetype instead of extension.
Simplify the code and avoids to deal with File paths directly.